fix: complete DOCX forward recovery - #237
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe RAGFlow DOCX parser now performs strict OPC and OOXML validation, rejects unsupported or malformed content with typed errors, and extracts source-ordered blocks while preserving text. Tests add hostile package fixtures, seam comparisons, compiler safety checks, and updated registration metadata. ChangesDOCX parser validation and extraction
Expired lease integration test
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant RAGFlowDocxParser
participant OPCValidator
participant PythonDocx
Compiler->>RAGFlowDocxParser: provide DOCX bytes
RAGFlowDocxParser->>OPCValidator: validate package and OOXML grammar
OPCValidator-->>RAGFlowDocxParser: admit package or raise typed refusal
RAGFlowDocxParser->>PythonDocx: construct document from validated bytes
PythonDocx-->>RAGFlowDocxParser: provide document text and tables
RAGFlowDocxParser-->>Compiler: return RawDocxBlock records
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
third_party/ragflow/deepdoc/parser/docx_parser.py (1)
925-931: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHold one copy of the decompressed package, not two.
raw_package_membersstores the full decompressed bytes of every member at line 927. Line 970 then reads each member again intopackage_members. In the common path no fallback runs, so the second copy is never used and peak memory is twice the decompressed package size.The runner bounds compressed artifact bytes only. Decompressed size is unbounded, so the duplicate retention doubles the exposure of a high-ratio archive.
Read the raw bytes only when a fallback path is entered, or reuse the bytes already held in
package_members. The same duplication exists inthird_party/ragflow/patches/issue-204-docx-parser.patchlines 1049-1053; keep both in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/ragflow/deepdoc/parser/docx_parser.py` around lines 925 - 931, The DOCX parsing flow retains duplicate decompressed package bytes in raw_package_members and package_members. Update the parser to read and retain member bytes only when the fallback path requires them, or reuse the existing package_members bytes, while preserving normal parsing behavior; apply the same change to the corresponding issue-204-docx-parser.patch implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/ragflow/deepdoc/parser/docx_parser.py`:
- Around line 1209-1237: The early return in
_contains_unrepresented_package_structure incorrectly stops scanning after an
empty unrepresented package part. In
third_party/ragflow/deepdoc/parser/docx_parser.py lines 1209-1237, change the
conditional to return True only when the element has content, then continue to
inspect later parts; apply the identical change in
third_party/ragflow/patches/issue-204-docx-parser.patch lines 1368-1369 so the
patch record matches the vendored implementation.
- Around line 741-753: Update _resolved_relationship_target in
third_party/ragflow/deepdoc/parser/docx_parser.py to process each .. by removing
one parent segment and raise only when resolution escapes the package root;
relax the corresponding .. validation near line 798. Apply the identical change
in third_party/ragflow/patches/issue-204-docx-parser.patch at lines 846-847. In
tests/unit/test_ragflow_document_compiler.py lines 101-105, remove the
_save_docx rewrite of Target="../customXml/item1.xml" and add coverage compiling
unmodified document.save() bytes.
In `@third_party/ragflow/MODIFICATIONS.md`:
- Line 72: Update the wording in MODIFICATIONS.md so the compound modifier reads
“non-XML-related parts” instead of “non-XML related parts,” preserving the
surrounding text.
---
Nitpick comments:
In `@third_party/ragflow/deepdoc/parser/docx_parser.py`:
- Around line 925-931: The DOCX parsing flow retains duplicate decompressed
package bytes in raw_package_members and package_members. Update the parser to
read and retain member bytes only when the fallback path requires them, or reuse
the existing package_members bytes, while preserving normal parsing behavior;
apply the same change to the corresponding issue-204-docx-parser.patch
implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69af4a75-d6e9-4d07-bddc-db56619fa99f
📒 Files selected for processing (8)
THIRD_PARTY_SBOM.cyclonedx.jsontests/integration/test_file_import_tracer.pytests/unit/test_ragflow_document_compiler.pytests/unit/test_third_party_ragflow_registration.pythird_party/ragflow/MODIFICATIONS.mdthird_party/ragflow/UPSTREAM.tomlthird_party/ragflow/deepdoc/parser/docx_parser.pythird_party/ragflow/patches/issue-204-docx-parser.patch
|
Review follow-up: I also accept the decompressed-package memory finding from the CodeRabbit review summary. The fix owner will remove duplicate retained member-byte copies without weakening duplicate-entry identity checks, add bounded regression evidence where practical, and keep the vendored source, patch record, and registration hashes synchronized. |
1 participant
Closes #204.
Summary
w:fldSimple,w:smartTag, package grammar, content-type, relationship, fixed-object, and duplicate-ZIP-entry bypassesZipInfoidentity so a benign first entry cannot mask unsafe bytes in a later entry with the same filenameVerification
make lintmake typecheckmake test: 2,818 passedmake catalog: 135 passed; 15 invariants / 12 fixtures; 2 third-party registrationsmake checkstopped ataction-buildbecause dependencies had not been installed;make installthen succeeded and the complete prepared rerun passedm0SecurityDecision=passNOT_PRODUCEDby maintainer decisionSummary by CodeRabbit
Bug Fixes
Quality Improvements